[SPARK-4631] unit test for MQTT #3844
Conversation
|
Can one of the admins verify this patch? |
|
@tdas verify this patch |
|
Jenksin, this is ok to test. |
|
Test build #24922 has started for PR 3844 at commit
|
There was a problem hiding this comment.
This can cause the SparkContext to be not shutdown if there is an exception in the unit test, causing a leaked SparkContext. Take a look at how it is done in the KafkaStreamSuite with BeforeAndAfter.
|
Test build #24922 has finished for PR 3844 at commit
|
|
Test FAILed. |
|
Test build #24946 has started for PR 3844 at commit
|
|
Test build #24948 has started for PR 3844 at commit
|
|
Test build #24949 has started for PR 3844 at commit
|
|
Test build #24946 has finished for PR 3844 at commit
|
|
Test FAILed. |
|
Test build #24948 has finished for PR 3844 at commit
|
|
Test FAILed. |
|
Test build #24949 has finished for PR 3844 at commit
|
|
Test FAILed. |
|
Test build #24956 has started for PR 3844 at commit
|
|
Test build #24956 has finished for PR 3844 at commit
|
|
Test FAILed. |
There was a problem hiding this comment.
nit: Why is there a String.valueOf(String)?
|
Test build #25007 has started for PR 3844 at commit
|
|
Test build #25007 has finished for PR 3844 at commit
|
|
Test PASSed. |
external/mqtt/pom.xml
Outdated
There was a problem hiding this comment.
Ummm.. isnt this new dependency necessary only for unit test (since nothing in non-test code changed)? In that case it should be added to test scope (See scalatest below). We really try to avoid changing dependencies as any such change can cause conflicts with other stuff (spark's code dependencies) causing unforeseen failures. So if this is only necessary for test, please put it in test scope.
|
Only one more comment. |
|
Test build #25035 has started for PR 3844 at commit
|
|
Test build #25035 has finished for PR 3844 at commit
|
|
Test PASSed. |
There was a problem hiding this comment.
nit: I missed this in the last pass, but this violates the Scala syntax that we follow. I wont block this PR for this.
There was a problem hiding this comment.
Can you explain what is the correction here. Just to understand what went wrong.
There was a problem hiding this comment.
for (...) {
msgTopic.publish(message)
}
Such code block should either be in one line or be within braces.
|
Merging this, thanks! |
Please review the unit test for MQTT Author: bilna <bilnap@am.amrita.edu> Author: Bilna P <bilna.p@gmail.com> Closes #3844 from Bilna/master and squashes the following commits: acea3a3 [bilna] Adding dependency with scope test 28681fa [bilna] Merge remote-tracking branch 'upstream/master' fac3904 [bilna] Correction in Indentation and coding style ed9db4c [bilna] Merge remote-tracking branch 'upstream/master' 4b34ee7 [Bilna P] Update MQTTStreamSuite.scala 04503cf [bilna] Added embedded broker service for mqtt test 89d804e [bilna] Merge remote-tracking branch 'upstream/master' fc8eb28 [bilna] Merge remote-tracking branch 'upstream/master' 4b58094 [Bilna P] Update MQTTStreamSuite.scala b1ac4ad [bilna] Added BeforeAndAfter 5f6bfd2 [bilna] Added BeforeAndAfter e8b6623 [Bilna P] Update MQTTStreamSuite.scala 5ca6691 [Bilna P] Update MQTTStreamSuite.scala 8616495 [bilna] [SPARK-4631] unit test for MQTT (cherry picked from commit e767d7d) Signed-off-by: Tathagata Das <tathagata.das1565@gmail.com>
|
@tdas, Thanks. |
|
It looks like there's maybe a port-binding / racing issue here? |
|
ok.. I will look into it |
|
There also seems to be a race condition introduced by this test. It fails consistently for me (but passes if I add a |
|
@dragos good catch. It sounds like an issue with the test if anything. You could just reopen SPARK-4631 with a workaround. |
|
@srowen I commented on the ticket, but I can't re-open it. |
|
See #4270 |
modified to adhere to accepted coding standards as pointed by tdas in PR #3844 Author: prabs <prabsmails@gmail.com> Author: Prabeesh K <prabsmails@gmail.com> Closes #4178 from prabeesh/master and squashes the following commits: bd2cb49 [Prabeesh K] adress the comment ccc0765 [prabs] adress the comment 46f9619 [prabs] adress the comment c035bdc [prabs] adress the comment 22dd7f7 [prabs] address the comments 0cc67bd [prabs] adress the comment 838c38e [prabs] adress the comment cd57029 [prabs] address the comments 66919a3 [Prabeesh K] changed MqttDefaultFilePersistence to MemoryPersistence 5857989 [prabs] modified to adhere to accepted coding standards
8 participants
Please review the unit test for MQTT